Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] OTEL Observability #297

Closed
wants to merge 5 commits into from

Conversation

IshanDaga
Copy link
Member

@IshanDaga IshanDaga commented Sep 9, 2023

Feature

Using OTEL lib to instrument the server. Happy to take suggestions on things that we should track, metrics to add, and how to allow maximum configurability for the Tracer.

Tasks

  • Add context object to each packet
  • Add OTEL config options to server.Options struct
  • Add spans for client events
  • Add metrics tracking using OTEL Meters API
  • Instrument slog handler

Pending decisions

  • should we add another external dependency like https://github.com/ttys3/slogx to internally instrument slog, or should we leave that to the user to implement within their own hook implementations ?

@IshanDaga IshanDaga added the enhancement New feature or request label Sep 9, 2023
@IshanDaga
Copy link
Member Author

@dgduncan @mochi-co
Another approach to this could be to have a StartTrace hook that is called after receivePacket
We could also add in a trace-modifier hook of sorts, that allows someone to insert the TraceID into the header for MQTTv5 or modify the packet to include TraceID bytes with a separator like in #227

Looking for initial comments and thoughts :)

@IshanDaga
Copy link
Member Author

@mochi-co hey I'm having trouble with the test cases involving sample packets.
Most are failing on the require.Equal since now a packet from the server has a context, but the sample packet doesn't.

There's quite a few of these cases, any suggestions on how I could move past this without breaking the current testing method ?

@mochi-co
Copy link
Collaborator

@IshanDaga SImplest solution would be to only populate context if OTEL tracing is enabled:

Index: clients.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/clients.go b/clients.go
--- a/clients.go	(revision 9d6c78915853d616b25757776b6945885b48df22)
+++ b/clients.go	(date 1697972510469)
@@ -454,7 +454,9 @@
 func (cl *Client) ReadPacket(fh *packets.FixedHeader) (pk packets.Packet, err error) {
 	atomic.AddInt64(&cl.ops.info.PacketsReceived, 1)
 
-	pk.Ctx, pk.Cancel = context.WithCancel(cl.State.open)
+	if cl.ops.options.OTELTracing {
+		pk.Ctx, pk.Cancel = context.WithCancel(cl.State.open)
+	}
 
 	pk.ProtocolVersion = cl.Properties.ProtocolVersion // inherit client protocol version for decoding
 	pk.FixedHeader = *fh

@IshanDaga
Copy link
Member Author

@mochi-co makes sense, thanks !

@mochi-co
Copy link
Collaborator

@IshanDaga This PR is a bit out of date, I recommend closing it and opening a new one when there's more changes 👍🏻

@IshanDaga
Copy link
Member Author

@mochi-co makes sense, closing for now, will get back on this as soon as i have a little more free time

@IshanDaga IshanDaga closed this May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants